fix(storage): reclaim upgrade residue safely - #4922
Conversation
Resolve upgrade orphan paths through the same removal identity used by ordinary artifact purges. Keep out-of-root entries pending and discharge aliases of live artifacts without deleting their bytes. Fixes apache#4910 Generated-by: Codex
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head 9052cb808cb7a5af0f8185b8228afe2b05b66286.
I found no P0–P3 correctness, ownership, race, path-containment, or cleanup-durability issue in the two-file diff. packages/storage/src/artifact-store.ts:484-526 resolves upgrade residue through real parents and entries, unlinks symlinks without following them, enforces real artifact-root containment, and protects current records through comparison identities. The tests cover an externally replaced parent directory, a case-insensitive live-path alias, and partial cleanup with retry persistence.
The exact-head hosted label, test, and windows_recovery checks passed. I did not independently run the Storage typecheck or tests because this checkout has no usable local TypeScript executable, and I did not run an additional filesystem fault-injection smoke test.
Automated review notice: This is an AI-assisted review and does not replace independent human review.
…residue-4910 # Conflicts: # packages/storage/src/artifact-store.ts
Keep the upstream paginated maintenance contract while resolving orphan entries through filesystem identity and querying only plausible live aliases.\n\nGenerated-by: Codex
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the focused fix. Reviewed 96412b7e: I found no P0–P3 issues in the upgrade-residue reclamation scope. Reusing the existing removal-identity and real-parent containment checks addresses the shared cause without introducing another file-identity definition. Unsafe or failed cleanup retains its note.
Seven targeted upgrade/orphan checks passed. Replacing the two production owners with the base versions makes the new parent-replacement and case-alias protections fail. I agree with the issue's calibrated scope: this is latent invariant hardening, not evidence that normal released clients are currently losing data. Current head is unchanged, checks pass and GitHub reports MERGEABLE.
AI-assisted review with an independent storage reviewer and primary-agent reconciliation; no full-repository suite was rerun.
简体中文
感谢这个范围克制的修复。基于 96412b7e，升级残留清理范围内未发现 P0–P3。复用现有删除身份与真实父目录包含检查，修复共同根因，没有另建文件身份定义；不安全或失败时保留清理记录。
七项定向检查通过；替换回 base 的两个生产 owner，新增父目录替换和大小写别名保护会失败。同意 issue 的定级：这是潜在不变量加固，不代表正常发布客户端正在丢数据。当前 head 未变，检查通过、MERGEABLE。
AI 辅助评审，独立 storage reviewer 后由主审复核；未重跑全仓测试。
2 participants
Summary
Fixes #4910
Verification
npm --workspace @maka/storage run buildNODE_NO_WARNINGS=1 npm --workspace @maka/storage run test:dist— 1129 passed, 8 platform skips, 0 failednpm --workspace @maka/storage run typechecknpx biome lint packages/storage/src/artifact-store.ts packages/storage/src/__tests__/artifact-stores.test.tsnpx biome format packages/storage/src/artifact-store.ts packages/storage/src/__tests__/artifact-stores.test.tsgit diff --checkNote: without
NODE_NO_WARNINGS=1, the existing cross-process managed-dependency test treats Node's SQLite experimental warning on stderr as an error. The isolated failure reproduces independently of this change.AI use
Tool(s) and scope: Codex implemented the fix, added regression tests, ran verification, and reviewed the diff against the issue and repository standards. The commit includes the required
Generated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?